-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
i18n: rewrite dom-size
description to be more i18n friendly
#9690
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with two additional suggestions. It took me a while to realize children/parent element
probably means children per parent element
and not children or parent element
:)
@@ -27,8 +27,8 @@ const UIStrings = { | |||
failureTitle: 'Avoid an excessive DOM size', | |||
/** Description of a Lighthouse audit that tells the user *why* they should reduce the size of the web page's DOM. The size of a DOM is characterized by the total number of DOM elements and greatest DOM depth. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */ | |||
description: 'Browser engineers recommend pages contain fewer than ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: 'Browser engineers recommend pages contain fewer than ' + | |
description: 'Browser engineers recommend that pages contain fewer than ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm not sure why the diff is showing currently three spaces and this is deleting one when it's clearly going 2->2, but I guess you can't just accept these changes cause it would need collect-strings anyway :)
`~${MAX_DOM_ELEMENTS.toLocaleString()} DOM elements. The sweet spot is a tree ` + | ||
`depth < ${MAX_DOM_TREE_DEPTH} elements and fewer than ${MAX_DOM_TREE_WIDTH} ` + | ||
`~${MAX_DOM_ELEMENTS.toLocaleString()} DOM elements. A more ideal layout is a tree ` + | ||
`depth less than ${MAX_DOM_TREE_DEPTH} elements and fewer than ${MAX_DOM_TREE_WIDTH} ` + | ||
'children/parent element. A large DOM can increase memory usage, cause longer ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'children/parent element. A large DOM can increase memory usage, cause longer ' + | |
'children per parent. A large DOM can increase memory usage, cause longer ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another rewording suggested
@@ -27,8 +27,8 @@ const UIStrings = { | |||
failureTitle: 'Avoid an excessive DOM size', | |||
/** Description of a Lighthouse audit that tells the user *why* they should reduce the size of the web page's DOM. The size of a DOM is characterized by the total number of DOM elements and greatest DOM depth. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */ | |||
description: 'Browser engineers recommend pages contain fewer than ' + | |||
`~${MAX_DOM_ELEMENTS.toLocaleString()} DOM elements. The sweet spot is a tree ` + | |||
`depth < ${MAX_DOM_TREE_DEPTH} elements and fewer than ${MAX_DOM_TREE_WIDTH} ` + | |||
`~${MAX_DOM_ELEMENTS.toLocaleString()} DOM elements. A more ideal layout is a tree ` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont like using the term layout like this.
`~${MAX_DOM_ELEMENTS.toLocaleString()} DOM elements. A more ideal layout is a tree ` + | |
`~${MAX_DOM_ELEMENTS.toLocaleString()} DOM elements. Ideally, the DOM tree's` + |
`~${MAX_DOM_ELEMENTS.toLocaleString()} DOM elements. The sweet spot is a tree ` + | ||
`depth < ${MAX_DOM_TREE_DEPTH} elements and fewer than ${MAX_DOM_TREE_WIDTH} ` + | ||
`~${MAX_DOM_ELEMENTS.toLocaleString()} DOM elements. A more ideal layout is a tree ` + | ||
`depth less than ${MAX_DOM_TREE_DEPTH} elements and fewer than ${MAX_DOM_TREE_WIDTH} ` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`depth less than ${MAX_DOM_TREE_DEPTH} elements and fewer than ${MAX_DOM_TREE_WIDTH} ` + | |
` maximum depth is less than ${MAX_DOM_TREE_DEPTH} elements and each parent element has fewer than ${MAX_DOM_TREE_WIDTH} ` + |
`~${MAX_DOM_ELEMENTS.toLocaleString()} DOM elements. The sweet spot is a tree ` + | ||
`depth < ${MAX_DOM_TREE_DEPTH} elements and fewer than ${MAX_DOM_TREE_WIDTH} ` + | ||
`~${MAX_DOM_ELEMENTS.toLocaleString()} DOM elements. A more ideal layout is a tree ` + | ||
`depth less than ${MAX_DOM_TREE_DEPTH} elements and fewer than ${MAX_DOM_TREE_WIDTH} ` + | ||
'children/parent element. A large DOM can increase memory usage, cause longer ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% agree with brendan's edit, but this would be edit given my rewording.
'children/parent element. A large DOM can increase memory usage, cause longer ' + | |
'children. A large DOM can increase memory usage, cause longer ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW in #9725 I also just proposed nuking these specific numbers entirely if that somehow helps the i18nness at the same time given our mismatch to scoring :)
+1 I don't think that these numbers are really as concrete as we present. Would be nice to remove/have new numbers/have them confirmed. |
Summary
tc/ came back with a concern about the language in
dom-size
, namely about "sweet spot" being toen-US
centric.